Move `LockableScore` requirement away from `Router` trait #1694

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1694      +/-   ##
==========================================
+ Coverage   90.92%   92.54%   +1.62%     
==========================================
  Files          85       86       +1     
  Lines       46268    59531   +13263     
  Branches    46268    59531   +13263     
==========================================
+ Hits        42067    55091   +13024     
- Misses       4201     4440     +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning-invoice/src/payment.rs

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

lightning-invoice/src/payment.rs

jkczyz · 2022-09-12T17:29:02Z

lightning-invoice/src/payment.rs

@@ -1860,6 +1842,8 @@ mod tests {

 					// Since the path is reversed, the last element in our iteration is the first
 					// hop.
+					let mut locked_scorer = self.scorer.lock();
+					let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

lightning-invoice/src/utils.rs

jkczyz · 2022-09-12T17:46:23Z

lightning-invoice/src/utils.rs

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+		self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+		self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+		self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+		self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

lightning-invoice/src/utils.rs

lightning-invoice/src/payment.rs

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

lightning-invoice/src/payment.rs

tnull

Generally LGTM

lightning-invoice/src/payment.rs

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

lightning-invoice/src/payment.rs

 //! # use secp256k1::PublicKey;
 //! # use std::cell::RefCell;
 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:

diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46 Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ce Compare September 11, 2022 22:01

tnull reviewed Sep 12, 2022

View reviewed changes

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Sep 12, 2022

View reviewed changes

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

jkczyz reviewed Sep 12, 2022

View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022

View reviewed changes

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

jkczyz reviewed Sep 13, 2022

View reviewed changes

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

jkczyz reviewed Sep 14, 2022

View reviewed changes

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

jkczyz reviewed Sep 14, 2022

View reviewed changes

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

jkczyz previously approved these changes Sep 14, 2022

View reviewed changes

TheBlueMatt reviewed Sep 14, 2022

View reviewed changes

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

lightning-invoice/src/payment.rs Show resolved Hide resolved

jurvis dismissed jkczyz’s stale review via b194c1d September 15, 2022 02:40

tnull reviewed Sep 15, 2022

View reviewed changes

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46ed Compare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022

View reviewed changes

tnull reviewed Sep 16, 2022

View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3e September 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3e Compare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022

View reviewed changes

jkczyz approved these changes Sep 16, 2022

View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:main Sep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022

[0.0.111-bindings] Bindings Branch Rebase + Backports #1730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `LockableScore` requirement away from `Router` trait #1694

Move `LockableScore` requirement away from `Router` trait #1694

jurvis commented Sep 5, 2022 •

edited

Loading

jurvis commented Sep 5, 2022

TheBlueMatt commented Sep 6, 2022

jkczyz commented Sep 6, 2022

jurvis commented Sep 7, 2022

jurvis commented Sep 10, 2022

jurvis commented Sep 10, 2022

codecov-commenter commented Sep 10, 2022 •

edited

Loading

TheBlueMatt left a comment

jkczyz Sep 12, 2022

jkczyz Sep 12, 2022

jurvis Sep 13, 2022

TheBlueMatt Sep 13, 2022

tnull left a comment

jkczyz left a comment

TheBlueMatt left a comment

tnull left a comment

jkczyz commented Sep 15, 2022

jurvis commented Sep 15, 2022

tnull left a comment

tnull Sep 16, 2022

jurvis Sep 16, 2022

Move LockableScore requirement away from Router trait #1694

Move LockableScore requirement away from Router trait #1694

Conversation

jurvis commented Sep 5, 2022 • edited Loading

Purpose

Implementation

jurvis commented Sep 5, 2022

TheBlueMatt commented Sep 6, 2022

jkczyz commented Sep 6, 2022

jurvis commented Sep 7, 2022

jurvis commented Sep 10, 2022

jurvis commented Sep 10, 2022

codecov-commenter commented Sep 10, 2022 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

jkczyz Sep 12, 2022

Choose a reason for hiding this comment

jkczyz Sep 12, 2022

Choose a reason for hiding this comment

jurvis Sep 13, 2022

Choose a reason for hiding this comment

TheBlueMatt Sep 13, 2022

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

jkczyz commented Sep 15, 2022

jurvis commented Sep 15, 2022

tnull left a comment

Choose a reason for hiding this comment

tnull Sep 16, 2022

Choose a reason for hiding this comment

jurvis Sep 16, 2022

Choose a reason for hiding this comment

Move `LockableScore` requirement away from `Router` trait #1694

Move `LockableScore` requirement away from `Router` trait #1694

jurvis commented Sep 5, 2022 •

edited

Loading

codecov-commenter commented Sep 10, 2022 •

edited

Loading